Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@apps/dashboard-api/src/controllers/admin.metrics.controller.js`:
- Around line 55-57: The catch blocks in admin.metrics.controller.js currently
return raw err.message to clients (e.g., the catch that sends
res.status(500).json({ success: false, data: {}, message: err.message }));
change each catch to log the full error internally (use the existing logger or
console.error) and return a generic client-facing message like "Internal server
error" or "An unexpected error occurred" while preserving success:false and
data:{}; apply this pattern to every catch in this file (the ones referenced in
the review) so controllers such as the admin metrics handlers never expose
MongoDB/internal error details to clients.
- Around line 147-166: checkDayRetention currently issues a
DeveloperActivity.findOne per signup causing N+1 queries; instead, for a given
daysAfter compute the per-cohort target and next UTC date boundaries, collect
all developerIds from signups, run a single DeveloperActivity.find (or
aggregate) with { developerId: { $in: developerIds }, date: { $gte: target, $lt:
next } } and count distinct developerId results to produce retained; update
checkDayRetention to accept signups and daysAfter and return the deduplicated
count (use DeveloperActivity.distinct or an aggregation pipeline) so you perform
one DB query per retention day rather than one per signup.

In `@apps/dashboard-api/src/controllers/analytics.controller.js`:
- Around line 318-320: Replace the direct exposure of err.message in the catch
block inside the analytics controller (the catch after the function that sends
analytics response) by wrapping the original error in the AppError class and
delegating to the global error handler instead of sending raw error text;
specifically, construct a new AppError with a safe client-facing message (e.g.,
"Failed to fetch analytics") and an appropriate status code, attach the original
error as metadata if needed, and call next(new AppError(...)) (or pass it into
the centralized error handler) rather than doing res.status(500).json({...,
message: err.message}).
- Around line 274-276: In the analytics controller catch block that currently
does res.status(500).json({ success: false, data: {}, message: err.message }),
stop exposing err.message; import/use the AppError class and replace that
response with forwarding a sanitized AppError to Express (e.g. next(new
AppError('Internal server error', 500))) and log the original err to server logs
(console.error or the existing logger) so internal MongoDB details aren't
returned to clients.
- Around line 159-161: The catch block in analytics.controller is exposing raw
err.message to clients; instead import and use the AppError class and the
Express error flow: replace res.status(500).json({...err.message}) with logging
the original error (e.g., logger.error(err) or console.error(err)) and call
next(new AppError('Internal server error', 500)); ensure the controller function
signature accepts next and add the AppError import (AppError) so no
MongoDB/internal messages are returned to clients.
- Around line 213-215: The catch block in the analytics controller currently
sends err.message to the client (res.status(500).json(...)), exposing internal
errors; replace this with creation/forwarding of an AppError so clients only
receive a generic message. Inside the catch, log the original err for server
diagnostics, then call next(new AppError('Internal server error', 500)) (or
construct an AppError and pass to next) instead of using err.message; reference
the catch's err variable, res usage and the AppError class to implement this
change in the analytics controller method.

In `@apps/dashboard-api/src/controllers/events.controller.js`:
- Around line 25-27: The 400 response in the events controller returns {
success, message } but must include a data field per guidelines; update the
handler that checks the event variable (the block validating "event" and calling
res.status(400).json(...)) to return { success: false, data: {}, message: 'event
name is required' } instead of the current shape so all responses include the
required data key.
- Around line 31-36: The JSON error response returned when
ALLOWED_FRONTEND_EVENTS does not contain normalizedEvent must follow the `{
success: bool, data: {}, message: "" }` order; update the response in the
controller (the block that checks `ALLOWED_FRONTEND_EVENTS.has(normalizedEvent)`
and calls `res.status(400).json(...)`) to return `success: false`, `data: {}`
and `message: "Unknown event: \"...\". Allowed: ..."` in that exact order while
keeping the 400 status and the same message content.
- Around line 47-50: The catch block in the events controller currently logs the
raw error and sends a raw 500 response; replace this with the AppError pattern:
import AppError from '@urbackend/common' and in the catch handler do not expose
err to the client—log it internally if needed but call next(new
AppError('Internal server error', 500)) (or throw new AppError(...)) instead of
res.status(500).json(...); ensure the unique catch block in events.controller
(the handler that currently does console.error('[events.controller] track
error:', err)) uses AppError and does not include err details in the
client-facing message.

In `@apps/public-api/src/middlewares/api_usage.js`:
- Around line 77-103: The NX Redis set for the activation flag (flagKey / the
redis.set call inside the setImmediate block) currently writes a permanent key;
change it to include a 2-year TTL so the key is set NX with expiry (e.g., use
redis.set(flagKey, '1', 'NX', 'EX', 63072000) or equivalent setnx + expire
sequence if your Redis client requires different args) to prevent re-emission
after a Redis flush or eviction.

In `@apps/web-dashboard/src/components/Dashboard/DeveloperMetrics.jsx`:
- Around line 21-31: The component currently swallows fetch errors and returns
null; add an error state (e.g., error, setError) in the DeveloperMetrics
component, set setError(err) in the catch of fetchMetrics (and clear error
before retries), and update the render logic so that when error is truthy you
render a user-facing error message and a retry control that calls fetchMetrics
again; keep using the existing loading, metrics, setLoading semantics so loading
still shows while fetching and metrics renders when successful.

In `@apps/web-dashboard/src/pages/AdminMetrics.jsx`:
- Around line 88-90: The catch block in AdminMetrics.jsx uses brittle string
matching on e.message to decide to call navigate('/dashboard') or setError;
change it to check a structured error property (e.g., e.code ===
'ADMIN_REQUIRED') first and fall back to the existing message check, so replace
the current string-only test with a check for a standardized error code (and
only then fallback to message includes), updating the handling around navigate
and setError accordingly; coordinate with the backend to return an error object
with a code (e.g., 'ADMIN_REQUIRED') so the front-end can reliably branch on
e.code while preserving the current fallback behavior.

In `@packages/common/src/models/PlatformEvent.js`:
- Around line 55-58: The TTL index on platformEventSchema is correct but we
should add a clarifying comment and safeguard to prevent accidental expiry
surprises: update the PlatformEvent.js near platformEventSchema.index to
document that expireAfterSeconds is calculated from the document's timestamp
field value (which defaults to Date.now) and warn that any explicit timestamp
overrides (e.g., in emitEvent.js and reliabilityAlertQueue.js where timestamp:
new Date() is set) will affect expiry; also ensure the schema keeps default:
Date.now and consider adding a unit test or runtime assertion in
emitEvent.js/reliabilityAlertQueue.js that timestamp is not set to a past date
before insert.

In `@packages/common/src/queues/activityRollupQueue.js`:
- Around line 64-78: The aggregation built by Log.aggregate currently computes
apiCallCount, mailCount, and storageCount but omits webhooks; add a
webhookTriggeredCount field to the pipeline (similar to mailCount/storageCount)
using a $sum with a $cond and $regexMatch on the request path (e.g., match
"/api/webhook" or "/api/webhooks" as your routes use) so webhook calls are
counted between dayStart and dayEnd, then update the downstream mapping logic
that reads logAgg results into the developer map to use this new
webhookTriggeredCount value instead of leaving webhookTriggeredCount initialized
to 0.

---

Outside diff comments:
In `@apps/dashboard-api/src/controllers/analytics.controller.js`:
- Line 115: The controller currently returns raw data via
res.json(formattedLogs); update the handler (the function that sends
formattedLogs in apps/dashboard-api/src/controllers/analytics.controller.js) to
wrap the payload in the standard envelope by returning res.json({ success: true,
data: formattedLogs, message: "" }) for successful responses (and similarly use
{ success:false, data:{}, message: "..." } for error paths) so all endpoints
conform to the `{ success: bool, data: {}, message: "" }` API contract.
- Around line 86-88: The catch currently returns err.message to the client;
instead log the original err internally (e.g., console.error(err)) and replace
the response with an AppError instance: create and pass new AppError('Internal
server error', 500) to the Express error handler via next(new AppError(...))
(ensure the controller signature includes next), removing any use of err.message
in res.status(...).json and keeping only a generic message to the client.
- Around line 116-118: In the catch block of the analytics controller replace
the direct response that exposes err.message with error-handling that uses the
AppError class and the route's next() so the centralized error middleware
formats the response as { success:false, data:{}, message:"" }; specifically,
remove res.status(500).json({ error: err.message }) and instead log the original
err (e.g., using console.error or processLogger.error) and call next(new
AppError(500, "Internal Server Error")) so no MongoDB/internal messages are sent
to the client and the global error handler returns the standardized payload.

In `@apps/dashboard-api/src/controllers/auth.controller.js`:
- Around line 161-189: When reconciling an existing Developer in the GitHub flow
(the branch that finds Developer via Developer.findOne and sets
developer.githubId/githubUsername/avatarUrl/isVerified), check the previous
isVerified value and, if it was false, call emitEvent(developer._id,
'email_verified', { method: 'github' }) after saving (or immediately before
returning) so the verification funnel is recorded; update the block that assigns
developer.isVerified = true and saves in the function handling the GitHub
profile to conditionally emit this event when transitioning from unverified to
verified.

---

Nitpick comments:
In `@apps/dashboard-api/src/controllers/analytics.controller.js`:
- Line 244: The aggregation currently uses a $push of the array field
activeProjectIds which creates nested arrays and is memory-inefficient; update
the pipeline used in the analytics aggregation (the variable/array building the
Mongo pipeline in the analytics controller) to $unwind the activeProjectIds
field before the $group stage so each project id is emitted as a single value,
then replace the group accumulator allProjectIds: { $push: '$activeProjectIds' }
with allProjectIds: { $addToSet: '$activeProjectIds' } to collect unique project
IDs without nested arrays; if you require distinct activeDays instead of
counting unwound documents, adjust the activeDays calculation accordingly (e.g.,
use a separate $addToSet on the day field before counting).

In `@apps/dashboard-api/src/routes/admin.metrics.js`:
- Around line 14-22: Add an explicit admin-check middleware to the router so
admin-only routes use both authMiddleware and requireAdmin at the route level
(e.g., change router.get('/overview', authMiddleware, getOverview) to
router.get('/overview', authMiddleware, requireAdmin, getOverview) for all
listed routes like '/overview', '/activation-funnel', '/cohorts', etc. Also
update the requireAdmin implementation to follow project conventions by throwing
an AppError (or passing an AppError to next) and returning the controller
response envelope { success, data, message } on denial so the router-level guard
matches existing error/response handling.

In `@apps/public-api/src/middlewares/api_usage.js`:
- Around line 83-84: Remove the inline require inside the setImmediate callback
and hoist the module import to the top of the file by adding a top-level const {
Project, PlatformEvent } = require('@urbackend/common');; then update the
setImmediate callback to use the already-imported Project and PlatformEvent (the
occurrences around the setImmediate where proj is fetched with Project.findById
and any PlatformEvent usage). Ensure there are no other inline requires for
'@urbackend/common' left in this file.

In `@apps/public-api/src/utils/emitEvent.js`:
- Around line 1-26: The emitEvent helper is duplicated across services; move the
function (signature emitEvent(developerId, event, properties = {}, projectId =
null) that uses setImmediate and PlatformEvent.create) into the shared
`@urbackend/common` utils, re-export it from the common package's public index,
then update both app-level copies to import { emitEvent } from
'@urbackend/common' instead of declaring it locally; also remove the unused
Project import from the original file and ensure behavior (fire-and-forget,
never throwing, same log message) remains identical after relocation.

In `@apps/web-dashboard/src/pages/AdminMetrics.jsx`:
- Around line 105-106: Remove the unnecessary queueMicrotask wrappers inside the
two useEffect hooks: call load and loadCohort directly from their respective
useEffect callbacks instead of wrapping them in queueMicrotask; update the
effect bodies that reference load and loadCohort so they simply invoke load()
and loadCohort() (preserving the dependency arrays) to simplify execution and
make behavior easier to debug.

In `@packages/common/src/models/DeveloperActivity.js`:
- Around line 27-30: The numeric activity fields in DeveloperActivity
(apiCallCount, mailSentCount, storageUploadsCount, webhookTriggeredCount) need
non-negative validation; update the Mongoose schema for those fields to include
a min: 0 validator (or equivalent validation) so attempts to set negative values
are rejected, and keep the existing default: 0; ensure any rollup/`$inc` paths
that update these fields still rely on schema validation or add runtime checks
to prevent negative results when applying decrements.
- Around line 23-26: The activeProjectIds array in the DeveloperActivity
mongoose schema is unbounded and could grow large; add an application-level
limit and validation to prevent excessive growth by updating the
activeProjectIds path in DeveloperActivity.js to enforce a maximum array length
(e.g., via Mongoose's validate or maxlength option) and document the expected
max entries in the model comment/README; alternatively, if many project refs are
expected, move these IDs to a separate collection or paginated subdocument store
and update any functions that push/pop project IDs to respect the new limit and
surface a clear error when exceeded.

In `@packages/common/src/models/PlatformEvent.js`:
- Around line 38-41: The PlatformEvent model's properties field is currently
mongoose.Schema.Types.Mixed with no constraints; add validation to prevent
oversized or unexpected keys by: implement a custom validator on the properties
field in the PlatformEvent schema (or replace Mixed with a stricter
subdocument/schema) that enforces a max serialized size (e.g.,
JSON.stringify(properties).length <= X bytes) and optionally restricts allowed
top-level keys (whitelist) or depth, and update any create/update paths that set
properties to ensure they respect this validation and return clear errors;
reference the properties field in the PlatformEvent schema and the model
construction to locate where to add the validator or nested schema.
- Line 24: Remove the redundant single-field index on developerId: the model
currently defines an individual index { developerId: 1 } and also a compound
index { developerId: 1, event: 1, timestamp: -1 } (in PlatformEvent.js); drop
the single-field index declaration for developerId so queries can use the
compound index prefix and avoid extra storage and write overhead.
- Line 35: Remove the redundant single-field index on event in the PlatformEvent
model: locate where the schema/indexes are defined (the entries `{ event: 1 }`
and the compound `{ event: 1, timestamp: -1 }`) and delete the individual `{
event: 1 }` index so only the compound `{ event: 1, timestamp: -1 }` remains;
this reduces duplicate index overhead while preserving query performance via the
compound index prefix.